feat(xtask): 'changes' xtask #12089

epage · 2023-05-05T15:54:37Z

This will report what commits affect each publishable package and serves as an alternative to #12085.

CI can run this as cargo changes HEAD~ HEAD^2 and that will capture all of the commits within the PR (this is the range I used in committed).

There is still work needed to integrate this with an action; this is just a rough base-line implementation.

I originally tried to use cargo package --list to determine what files belong to what packages but that was taking too long for some reason (it works well in cargo release for my crates). We can either look into that in the future or do our own heuristics to filter out content.

This will report what commits affect each publishable package and serves as an alternative to rust-lang#12085. CI can run this as `cargo changes HEAD~ HEAD^2` and that will capture all of the commits within the PR (this is the range I used in `committed`). There is still work needed to integrate this with an action; this is just a rough base-line implementation. I originally tried to use `cargo package --list` to determine what files belong to what packages but that was taking too long for some reason (it works well in `cargo release` for my crates). We can either look into that in the future or do our own heuristics to filter out content.

rustbot · 2023-05-05T15:54:42Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

weihanglo

Thanks for the fast prototyping. This looks pretty complete to me!

Some thoughts:

The cargo project doesn't adopt git conventional commit message pattern. Not sure if it is worthy or we should enforce that.
Perhaps switch to gix if we plan to drop git2 someday.
If we merge this or else, should we remove xtask-unpublished?
The code is indeed in a high quality. I can't be pickier at code level. However, total lines of code usually correlate to maintenance cost. I am a bit worried it's a bit overkill when comparing to what can be done in shell scripts.
This is a good demonstration of how people can invent something interesting with cargo and other libraries. I love the pretty status output!

epage · 2023-05-05T17:28:03Z

The cargo project doesn't adopt git conventional commit message pattern. Not sure if it is worthy or we should enforce that.

I copied that over from cargo release changes and am willing to drop it. The intention with both is that this is meant to help raw attention to extra information when its present, not to be prescriptive about it. The way the code is structured, conventional_status reports whether it has something or not and status can then report other information if we want to support other conventions.

Perhaps switch to gix if we plan to drop git2 someday.

Yeah, I stuck with what I was used to / what there are existing resources for. Digging into gix, I think I can switch over to it; the main question is on rev_parse_single as the docs aren't too clear on the behavior.

If we merge this or else, should we remove xtask-unpublished?

Unsure, in part because these are experiments but in part because I don't fully remember what all building blocks I originally intended...

The code is indeed in a high quality. I can't be pickier at code level. However, total lines of code usually correlate to maintenance cost. I am a bit worried it's a bit overkill when comparing to what can be done in shell scripts.

Shell scripts are also deceptively short and error prone :)

I also went past the absolute minimum (e.g. defaulting base-sha and head-sha, less error prone practices like enums over strings) for local usage as an xtask. If we are concerned about the burden we are taking on, we can drop features to be more inline with the shell script.

At the moment, this is also written in a way that I could see it being useful for writing up changelogs, so we might be able to get more mileage out of it.

This is a good demonstration of how people can invent something interesting with cargo and other libraries. I love the pretty status output!

And it is another way for us to dogfood to identify problems, like the fact that I can't make a call to say "tell me what files will be included in a package" but have to shell out for it (except I dropped it due to performance issues I hadn't debugged yet).

This also reminds me that I hope to simplify some of this code in the future. For clap, I created anstream which is like fwdansi on steroids. If we adopted it within clap, we could ore easily write formatted text, rather than having to use the individual Shell::write_stderr calls. Instead, we could do Shell::status and the message line could include formatting and it'd Just Work under all circumstances.

bors · 2023-05-06T22:34:23Z

☔ The latest upstream changes (presumably #12096) made this pull request unmergeable. Please resolve the merge conflicts.

weihanglo · 2023-05-09T12:53:15Z

At the moment, this is also written in a way that I could see it being useful for writing up changelogs, so we might be able to get more mileage out of it.

This made me wonder we would like to change each PR title before r+. It will make more sense for conventional commit messages.

Unsure, in part because these are experiments but in part because I don't fully remember what all building blocks I originally intended...

My fault. I should ask your original intent first for xtask-unpublished but what I did was just guessing 😅.

To be clear, I see the values in these conventions and the command. However, it feels like going too far away from #12033. I was hoping there could be a way to catch not-yet-bumped versions in CI ASAP, so that we can start splitting internal building blocks into subcrates.

ehuss · 2023-05-11T18:57:29Z

r? weihanglo

I think Weihang is following this more closely than I am. I haven't really reviewed this in detail, but the general concept seems good.

weihanglo

This looks fine for listing changes. However, I still don't know how it works with requiring user to do a version bump. Did it really give user the information that they need bumping a version for a crate?

weihanglo · 2023-05-11T22:00:50Z

crates/xtask-changes/src/xtask.rs

+    paths: std::collections::BTreeSet<std::path::PathBuf>,
+}
+
+impl CommitDescription {


I would probably suggest to remove conventional commit part. Let's focus on the original goal for now.

weihanglo · 2023-05-11T22:04:17Z

crates/xtask-changes/src/xtask.rs

+        .arg(clap::Arg::new("base-sha").help("SHA to diff against"))
+        .arg(clap::Arg::new("head-sha").help("SHA with changes"))


Do you consider copying part of #12126 for CI integration?

weihanglo · 2023-05-11T22:27:15Z

[Not a blocker] I still lean toward merging a similar approach like #12126 as a immediate guard rail. We can then properly design and expand this automation, thinking more on what we want for contributors and CI.

ci: check if any version bump needed for member crates ### What does this PR try to resolve? Check if a crate requires a version bump in CI. Part of #12033 ### How should we test and review this PR? Demo action result: <https://github.com/weihanglo/cargo/actions/runs/4941952784/jobs/8835049007> ### Additional information This doesn't devalue #12089. I love the changelog detection, saving maintainers' times a lot ( I am the one writing changelogs for each release for now). However, the amount of code there is too excessive to me. I think someday when we are going to integrate [cargo-release](https://crates.io/crates/cargo-release) and/or [ehuss/cargo-new-release](https://github.com/ehuss/cargo-new-release), we can remove this script perhaps entirely :) I tried to document the script a bit hard. Hope it won't get worse over time.

epage · 2023-07-26T19:46:35Z

Based on the direction of #12395, I think we can close this. What information we can learn from here has been called out in that other PR.

rustbot assigned ehuss May 5, 2023

rustbot added A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2023

weihanglo reviewed May 5, 2023

View reviewed changes

weihanglo mentioned this pull request May 10, 2023

ci: check if any version bump needed for member crates #12126

Merged

rustbot assigned weihanglo and unassigned ehuss May 11, 2023

weihanglo reviewed May 11, 2023

View reviewed changes

epage closed this Jul 26, 2023

epage deleted the changes branch July 26, 2023 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(xtask): 'changes' xtask #12089

feat(xtask): 'changes' xtask #12089

epage commented May 5, 2023

rustbot commented May 5, 2023

weihanglo left a comment

epage commented May 5, 2023

bors commented May 6, 2023

weihanglo commented May 9, 2023

ehuss commented May 11, 2023

weihanglo left a comment

weihanglo May 11, 2023

weihanglo May 11, 2023

weihanglo commented May 11, 2023 •

edited

Loading

epage commented Jul 26, 2023

		.arg(clap::Arg::new("base-sha").help("SHA to diff against"))
		.arg(clap::Arg::new("head-sha").help("SHA with changes"))

feat(xtask): 'changes' xtask #12089

feat(xtask): 'changes' xtask #12089

Conversation

epage commented May 5, 2023

rustbot commented May 5, 2023

weihanglo left a comment

Choose a reason for hiding this comment

epage commented May 5, 2023

bors commented May 6, 2023

weihanglo commented May 9, 2023

ehuss commented May 11, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo May 11, 2023

Choose a reason for hiding this comment

weihanglo May 11, 2023

Choose a reason for hiding this comment

weihanglo commented May 11, 2023 • edited Loading

epage commented Jul 26, 2023

weihanglo commented May 11, 2023 •

edited

Loading